I would not expect start polling to return the gasEstimates since those will be updated in the state and we are following a reactive paradigm, the values should always be read from the state as it is the source of truth.

I would expect though that start polling returns the pollToken or a fresh pollToken in case the argument is falsy, consider this code:

// "Mounting / Unmounting / changing pollToken" effect
useEffect(() => {
  const token = getGasFeeEstimatesAndStartPolling(pollToken);
  setPollToken(token);
  return () => {
	if (token) {
      disconnectPoller(token);
    }
  }
}, [setPollToken, pollToken]);

this would work in the case pollToken is set or not:

const [pollToken, setPollToken] = useState(null);
// or 
const [pollToken, setPollToken] = useState(txHash);

In order to do so:

• start polling with an existing poll token is a no op
• start polling with a falsy poll token returns a new poll token

I'd encourage the use of this pattern because we minimize UI <-> Controllers polling issues.

Huh. So digits after the third are omitted 🤔

This seems fine, given that WEI isn't really divisible. But this is probably worth documenting at least, so it doesn't come as a surprise (e.g. "rounds down to the nearest WEI".).

yes, except I realize now that a GWEI is 1,000,000,000 WEI (10^9), and not just 1000 WEI (10^3) as this logic would imply.
🤦

changing to .toFixed(9) should fix that.

is there a reason to not use the LegacyGasPriceEstimate interface?

import { LegacyGasPriceEstimate } from './GasFeeController';

//...

export async function fetchLegacyGasPriceEstimates(): Promise<LegacyGasPriceEstimate> {

Good call, Resolved 53250a9

I think this should be a string 'undefined', so typeof block.baseFee !== 'undefined';

Also, from testing on Ropsten, I believe it should be block.baseFeePerGas?

done in 2bdcd7d and baseFeePerGas is what we used on extension's version of this code with success.

This seems to be failing: Invalid character in 0x5f48b0f7. Is weiHexToGweiDec expecting other format?

Maybe without the 0x?

aabfa3a good catch, added a stripHexPrefix in the conversion method and added a test

Should this be slow, average, fast?

I thought about it, but I think the synergy afforded of having gasPriceEstimates.low/medium/high on two of the three valid gas estimate types is better, and the mapping of slow/avg/fast can happen on app side.

Makes sense, thanks Brad!

I think we can use optional chaining here, if our project supports it:

if (!this?.ethQuery?.sendAsync) {

resolved :)

The descriptions of these it blocks need to change to reflect that the function is converting to GWEI

41cdace done here

Might be personal preference, but I think these tests can be clearer in intent, and also less prone to error, if the hard coded inputs and hard coded outputs are explicitly provided, as opposed to be calculated by other functions.

This is how I would write these tests, feel free to copy and paste:

describe('weiHexToGweiDec', () => {
  it('should convert a whole number to WEI', () => {
    const testData = [
      {
        input: '3b9aca00',
        expectedResult: 1,
      },
      {
        input: '1ca35f0e00',
        expectedResult: 123,
      },
      {
        input: '178411b200',
        expectedResult: 101,
      },
      {
        input: '11f5021b400',
        expectedResult: 1234,
      },
    ];
    testData.forEach(({ input, expectedResult }) => {
      expect(
        util.weiHexToGweiDec(input),
      ).toBe(expectedResult);
    });
  });

  it('should convert a number with a decimal part to WEI', () => {
    const testData = [
      {
        input: '4190ab00',
        expectedResult: 1.1,
      },
      {
        input: '1ca3f7a480',
        expectedResult: 123.01,
      },
      {
        input: '178420f440',
        expectedResult: 101.001,
      },
      {
        input: '11f71ed6fc0',
        expectedResult: 1234.567,
      },
    ];

    testData.forEach(({ input, expectedResult }) => {
      expect(
        util.weiHexToGweiDec(input),
      ).toBe(expectedResult);
    });
  });

  it('should convert a number < 1 to WEI', () => {
    const testData = [
      {
        input: '5f5e100',
        expectedResult: 0.1,
      },
      {
        input: '989680',
        expectedResult: 0.01,
      },
      {
        input: 'f4240',
        expectedResult: 0.001,
      },
      {
        input: '21cbbbc0',
        expectedResult: 0.567,
      },
    ];

    testData.forEach(({ input, expectedResult }) => {
      expect(
        util.weiHexToGweiDec(input),
      ).toBe(expectedResult);
    });
  });

  it('should work with 0x prefixed values', () => {
    expect(util.weiHexToGweiDec('0x5f48b0f7')).toBe('1.598599415');
  });
});

I was not going to comment about this, but jest has a test.each function which is my personal preference, either as string literal table or array table.

Example:

test.each`
  a    | b    | expected
  ${1} | ${1} | ${2}
  ${1} | ${2} | ${3}
  ${2} | ${1} | ${3}
`('returns $expected when $a is added $b', ({a, b, expected}) => {
  expect(a + b).toBe(expected);
});

Example from mobile:
https://github.com/MetaMask/metamask-mobile/blob/978b73d88a5208a7d5d74415af980a2f3a59403d/app/components/Base/Keypad/createKeypadRule.test.js

copy and pasted, just had to update the expectedResult to strings

3a42d98